Skip to content

Enhancement - Introduce pvcName input param in CNSUnregisterVolume #3658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enhancement - Introduce pvcName input param in CNSUnregisterVolume #3658

wants to merge 1 commit into from
+1,190 −520

Conversation

kolluria
Copy link
Contributor

@kolluria kolluria commented Oct 7, 2025 •
edited
Loading

What this PR does / why we need it

  • Introduces a new field called pvcName as part of the CNS Unregister Volume spec. As the users are inclined to refer to the volumes using the PVCs rather than the volume handles, this would improve the UX.
  • Adds validation rules using x-kubernetes-validations to ensure that only one of the volumeID or pvcName is specified to avoid ambiguity.
  • Updates the reconciler to use the informer cache to find out the PV and VolumeID when the PVC Name is supplied.
  • Updates the reconciler to handle delete events gracefully by carefully deciding whether or not to continue with volume unregistration
  • Optimises the reconciler logic to ignore events that do not increment the generation.
  • Optimises the usage calculation logic by failing fast when usages are detected.
  • Updates the reconciler to add a finalizer to the CR before reconciling to have control over deletion process
  • Updates the reconciler to process the delete events and remove the finalizer for graceful deletion.
  • Updates the reconciler to protect the PVC using a finalizer to gracefully reconciler in case of retries
  • Removes the code that retains the persistent volume as it's no longer required
  • Updates the reconciler to remove the finalizer on the PVC once unregistration is successful for graceful deletion of the PVC.
  • Adds/updates unit tests wherever necessary and applicable.

Testing done

Input Validation

When input is invalid 

---
apiVersion: cns.vmware.com/v1alpha1
kind: CnsUnregisterVolume
metadata:
  name: unreg-with-volume-id-and-pvc
spec:
  pvcName: my-pvc
  volumeID: 123e4567-e89b-12d3-a456-426614174000
  retainFCD: true
  forceUnregister: false
---
apiVersion: cns.vmware.com/v1alpha1
kind: CnsUnregisterVolume
metadata:
  name: unreg-with-neither-pvc-nor-volume-id
spec:
  retainFCD: true
  forceUnregister: false

Result: CR admission was rejected with the expected errors -

Error from server (Invalid): error when creating "invalid_spec.yaml": CnsUnregisterVolume.cns.vmware.com "unreg-with-volume-id-and-pvc" is invalid: spec: Invalid value: "object": Cannot specify both 'volumeID' and 'pvcName' at the same time. Please specify only one of them
Error from server (Invalid): error when creating "invalid_spec.yaml": CnsUnregisterVolume.cns.vmware.com "unreg-with-neither-pvc-nor-volume-id" is invalid: spec: Invalid value: "object": Either 'volumeID' or 'pvcName' must be specified, but not both

Workflow validation

  1. Created 3 PVCs vol-used-by-pod, vol-used-by-vm and vol-unused
  2. Invoked unregistration on vol-unused with the Volume ID
  3. Invoked unregistration on vol-used-by-pod with the PVC Name
  4. Invoked unregistration on vol-used-by-vm with the PVC Name and forceUnregister set to true
Input 

---
apiVersion: cns.vmware.com/v1alpha1
kind: CnsUnregisterVolume
metadata:
  name: vol-used-by-pod
  namespace: cns-unregister-volume
spec:
  pvcName: vol-used-by-pod
---
apiVersion: cns.vmware.com/v1alpha1
kind: CnsUnregisterVolume
metadata:
  name: vol-used-by-vm
  namespace: cns-unregister-volume
spec:
  pvcName: vol-used-by-vm
  forceUnregister: true
---
apiVersion: cns.vmware.com/v1alpha1
kind: CnsUnregisterVolume
metadata:
  name: vol-unused
  namespace: cns-unregister-volume
spec:
  volumeID: 77983527-0b74-44ef-926c-eaff27f9e9f3

Output 

apiVersion: v1
items:
- apiVersion: cns.vmware.com/v1alpha1
  kind: CnsUnregisterVolume
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"cns.vmware.com/v1alpha1","kind":"CnsUnregisterVolume","metadata":{"annotations":{},"name":"vol-unused","namespace":"cns-unregister-volume"},"spec":{"volumeID":"77983527-0b74-44ef-926c-eaff27f9e9f3"}}
    creationTimestamp: "2025-09-30T09:22:32Z"
    finalizers:
    - cns.vmware.com/unregister-volume
    generation: 1
    name: vol-unused
    namespace: cns-unregister-volume
    resourceVersion: "5901789"
    uid: 17520557-cae7-402a-9f8d-7d7f92f5d99c
  spec:
    volumeID: 77983527-0b74-44ef-926c-eaff27f9e9f3
  status:
    unregistered: true
- apiVersion: cns.vmware.com/v1alpha1
  kind: CnsUnregisterVolume
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"cns.vmware.com/v1alpha1","kind":"CnsUnregisterVolume","metadata":{"annotations":{},"name":"vol-used-by-pod","namespace":"cns-unregister-volume"},"spec":{"pvcName":"vol-used-by-pod"}}
    creationTimestamp: "2025-09-30T09:22:30Z"
    finalizers:
    - cns.vmware.com/unregister-volume
    generation: 1
    name: vol-used-by-pod
    namespace: cns-unregister-volume
    resourceVersion: "5901750"
    uid: 664edba0-73be-4e0a-8efb-66c09c31d709
  spec:
    pvcName: vol-used-by-pod
  status:
    error: |-
      volume 369f3388-2a68-4ed9-8c89-0ff85a87b337 cannot be unregistered because volume is in use by the following resources:
       Pods: pod
    unregistered: false
- apiVersion: cns.vmware.com/v1alpha1
  kind: CnsUnregisterVolume
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"cns.vmware.com/v1alpha1","kind":"CnsUnregisterVolume","metadata":{"annotations":{},"name":"vol-used-by-vm","namespace":"cns-unregister-volume"},"spec":{"forceUnregister":true,"pvcName":"vol-used-by-vm"}}
    creationTimestamp: "2025-09-30T09:22:31Z"
    finalizers:
    - cns.vmware.com/unregister-volume
    generation: 1
    name: vol-used-by-vm
    namespace: cns-unregister-volume
    resourceVersion: "5901800"
    uid: 5467d868-c7d1-4967-a2c2-6cd352361147
  spec:
    forceUnregister: true
    pvcName: vol-used-by-vm
  status:
    unregistered: true
kind: List
metadata:
  resourceVersion: ""

Observations:

  1. As expected, unregistration of vol-used-by-pod failed with appropriate error -
      volume 369f3388-2a68-4ed9-8c89-0ff85a87b337 cannot be unregistered because volume is in use by the following resources:
       Pods: pod
  1. As expected, unregistration of vol-used-by-vm succeeded due to application of force and the PVC/PV are in terminating state(it will be garbage collected once the VM is deleted) -
NAME             STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS                VOLUMEATTRIBUTESCLASS   AGE
vol-used-by-vm   Terminating   pvc-3e01c03c-fc2a-4fad-946e-c13b58429b95   4Gi        RWO            wcpglobal-storage-profile   <unset>                 9m31s
  1. As expected, unregistration of vol-unused was successful.

Special notes for your reviewer:

Release note:

Introduce pvcName input param in CNSUnregisterVolume spec
Optimise CNSUnregisterVolume reconcile workflow
Handle delete events gracefully

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 7, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolluria

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2025
@kolluria
Copy link
Contributor Author

kolluria commented Oct 7, 2025

/assign @kolluria

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 7, 2025
@kolluria kolluria force-pushed the unreg-delete-workflow branch from c642957 to 1a5b942 Compare October 8, 2025 14:04
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2025
@kolluria kolluria force-pushed the unreg-delete-workflow branch from 1a5b942 to feec22b Compare October 8, 2025 14:04
@kolluria kolluria marked this pull request as ready for review October 8, 2025 14:05
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2025
@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 8, 2025
@kolluria kolluria changed the title Unreg delete workflow Enhancement - Introduce pvcName input param in CNSUnregisterVolume Oct 8, 2025
@kolluria kolluria mentioned this pull request Oct 8, 2025
Closed
@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

ok-to-test-wcp

@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

ok-to-test-tkg

@kolluria kolluria force-pushed the unreg-delete-workflow branch from feec22b to 395d870 Compare October 8, 2025 14:16
@deepakkinni
Copy link
Collaborator

Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete
Jenkins Build #468

@deepakkinni
Copy link
Collaborator

Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete
Jenkins Build #446

@kolluria kolluria force-pushed the unreg-delete-workflow branch from 395d870 to 8570708 Compare October 8, 2025 14:48
@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

/retest-required

@kolluria
Enhancement - Introduce pvcName input param in CNSUnregisterVolume …
28da214 
…spec

Adds validation rules using x-kubernetes-validations to ensure that only one of the volumeID or pvcName is specified to avoid ambiguity.
Updates the reconciler to use the informer cache to find out the PV and VolumeID when the PVC Name is supplied.
Optimises the reconciler logic to ignore all such events that do not increment the generation.
Optimises the usage calculation logic by failing fast when usages are detected.
Updates the reconciler to add a finalizer to the CR before reconciling to have control over deletion process
Updates the reconciler to process the delete events and remove the finalizer for graceful deletion.
Updates the reconciler to protect the PVC using a finalizer to gracefully reconciler in case of retries
Removes the code that retains the persistent volume as it's no longer required
Updates the reconciler to remove the finalizer on the PVC once unregistration is successful for graceful deletion of the PVC.
Adds/updates unit tests wherever necessary and applicable.
@kolluria kolluria force-pushed the unreg-delete-workflow branch from 8570708 to 28da214 Compare October 8, 2025 17:09
@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete Jenkins Build #446

Ran 14 of 1840 Specs
SUCCESS! -- 14 Passed | 0 Failed | 0 Pending | 1826 Skipped | 0 Flaked

@kolluria
Copy link
Contributor Author

kolluria commented Oct 8, 2025

Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete Jenkins Build #468

Ran 6 of 1840 Specs
SUCCESS! -- 6 Passed | 0 Failed | 0 Pending | 1834 Skipped | 0 Flaked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chethanv28 chethanv28 Awaiting requested review from chethanv28

@divyenpatel divyenpatel Awaiting requested review from divyenpatel

@gnufied gnufied Awaiting requested review from gnufied

@xing-yang xing-yang Awaiting requested review from xing-yang

Assignees

@kolluria kolluria

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kolluria @k8s-ci-robot @deepakkinni